Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make the content-length header more RFC compliant #10353

Merged
merged 2 commits into from
Feb 14, 2021
Merged

Make the content-length header more RFC compliant #10353

merged 2 commits into from
Feb 14, 2021
+41 −2

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Feb 2, 2021 •
edited
Loading

RFC7230 says:

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request.

and

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send a Content-Length header field in any 2xx (Successful) response to a CONNECT request (Section 4.3.6 of [RFC7231]).

This PR updates HTTP::Server to better follow the RFC in regard to when the content-length header is included automatically. Previously it would be set to 0, regardless of the response status, if it was not already set.

This PR does NOT handle the CONNECT request case, this is much less common and can be handled via another PR in the future.

Sija
Sija reviewed Feb 2, 2021
View reviewed changes
src/http/server/response.cr Outdated Show resolved Hide resolved
@Blacksmoke16
Copy link
Member Author

Actually after sleeping on this, do we want to go a step further and actually remove the content-length header if its present in these contexts versus just not automatically adding it? I.e. if an application sets it themselves, with a 204 they would still be in violation of the RFC.

@straight-shoota
Copy link
Member

straight-shoota commented Feb 2, 2021 •
edited
Loading

Let's leave the scope of this PR as is. That idea could be a follow-up but may need some more discussion first.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking labels Feb 3, 2021
@asterite asterite merged commit 152e390 into crystal-lang:master Feb 14, 2021
@Blacksmoke16 Blacksmoke16 deleted the 304-fix branch February 14, 2021 14:55
@bcardiff bcardiff added this to the 1.0.0 milestone Mar 10, 2021
@Blacksmoke16 Blacksmoke16 mentioned this pull request Mar 23, 2021
Closed
@Blacksmoke16 Blacksmoke16 mentioned this pull request Oct 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Blacksmoke16 @straight-shoota @Sija @asterite @bcardiff